Related to comments in containers/podman#25922

On the krunkit side, my thought was we could allow the user to still do --nested, but if krun_check_nested_virt returns <1 we would just ignore the argument and print something to stdout or log it describing as such

If it's not supported, couldn't we just have krun_set_nested_virt return an error code?

If it's not supported, couldn't we just have krun_set_nested_virt return an error code?

That was the original behavior. At this moment, if the user wants to enable nested virt, they either have to already know the requirements for the feature and do some system queries, or they have to try and call krun_set_nested_virt then check if it fails. This provides an easy way for the consumer of the API to programmatically check if they meet all necessary requirements before hand. Think of it like calling KVM_CHECK_EXTENSION on a specific capability before using the associated ioctl.

For consumers of krunkit, like Podman, having the command fail is not a graceful way to handle the issue. It's also not ideal UX to have each consumer do the same manual checks when we can provide it for them.

Your stated use-case in krunkit (the main consumer of this API):

On the krunkit side, my thought was we could allow the user to still do --nested, but if krun_check_nested_virt returns <1 we would just ignore the argument and print something to stdout or log it describing as such

What is the difference between krun_check_nested_virt returning <1 as opposed to krun_set_nested_virt returning <1. The outcome would be the same IMO.

For consumers of krunkit, like Podman, having the command fail is not a graceful way to handle the issue. It's also not ideal UX to have each consumer do the same manual checks when we can provide it for them.

We can define this failure as its own error code and have krunkit wrap/handle it accordingly for podman.

Your stated use-case in krunkit (the main consumer of this API):

On the krunkit side, my thought was we could allow the user to still do --nested, but if krun_check_nested_virt returns <1 we would just ignore the argument and print something to stdout or log it describing as such

What is the difference between krun_check_nested_virt returning <1 as opposed to krun_set_nested_virt returning <1. The outcome would be the same IMO.

In this scenario, you're right and the outcome would be the same. In the grand scheme of things, in my opinion, checking if nested virt is supported and enabling nested virt should be two separate operations.

I don't want this to be a controversial change, so if it's best to go about this a different way so everyone's happy I can do that.

I think the check itself is fine. I'm just not convinced it warrants its own krun API, when krun_set_nested_virt could do the check itself and return EOPNOTSUP if it fails. krunkit could then ignore the arg and print some log message saying its not supported.

I try to avoid adding new krun APIs unless absolutely necessary though. Perhaps @slp has different thoughts?

In this scenario, you're right and the outcome would be the same. In the grand scheme of things, in my opinion, checking if nested virt is supported and enabling nested virt should be two separate operations.

But every time you check for nested virt and it is supported, you would then enable, correct? This suggests to me it can be one operation.

I don't have a strong opinion either way, but for me having this API seems fine (and doesn't seem hard to maintain). But yes, you can get by without having this API.

This would perhaps be useful if you are creating a VM configuration to save to a file using some sort of CLI, but not starting it the VM yet - you can tell the user nested virt is not supported on their system.

This would perhaps be useful if you are creating a VM configuration to save to a file using some sort of CLI, but not starting it the VM yet - you can tell the user nested virt is not supported on their system.

In this scenario, that VM configuration would only apply to the system you're running on. You wouldn't be able to pick up a config file built on a M3 (that would indicate nested virt is supported) and use it on a M2 system (it would wrongly indicate that nested virt is supported).

Maybe I'm thinking too much into it, however. I just think that this check is best served when actually wanting to enable nested virt.

I will just remove the UAPI and put the nested check at the start of krun_set_nested_virt. If it fails I'll return EOPNOTSUPP.

I'd like to get this moving forward and get nested support in Podman

Sorry for chiming it late. Both options are reasonable, the problem is that krun_set_nested_virt was already part of a release, so we shouldn't change its behavior. It's true that, most likely, krunkit is the only user of this particular API, but changing it sets a bad precedent IMO.

In addition to this, most of the configuration calls we have right now doesn't check if the parameters are viable (they only they're valid in the most basic sense). This is something that we might consider changing in v2, when we revamp the API.

Sorry for chiming it late. Both options are reasonable, the problem is that krun_set_nested_virt was already part of a release, so we shouldn't change its behavior. It's true that, most likely, krunkit is the only user of this particular API, but changing it sets a bad precedent IMO.

This is something I didn't think about, but that's a really good point and I agree.

In addition to this, most of the configuration calls we have right now doesn't check if the parameters are viable (they only they're valid in the most basic sense). This is something that we might consider changing in v2, when we revamp the API.

Sorry for chiming it late. Both options are reasonable, the problem is that krun_set_nested_virt was already part of a release, so we shouldn't change its behavior. It's true that, most likely, krunkit is the only user of this particular API, but changing it sets a bad precedent IMO.

In addition to this, most of the configuration calls we have right now doesn't check if the parameters are viable (they only they're valid in the most basic sense). This is something that we might consider changing in v2, when we revamp the API.

Sounds good to me. Perhaps we'll re-examine this in v2.

Confirmed to work as expected in M1 (detects that nested virt is not available) and M3 (detects that nested virt is available).